Skip to content

fix #57749, model ccall binding access in inference #58872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix #57749, model ccall binding access in inference #58872

wants to merge 1 commit into from

Conversation

JeffBezanson
Copy link
Member

I've played around with ccall semantics a bunch (#57931) but concluded that a bandaid like this is far better for 1.12.

fixes #57749

@JeffBezanson JeffBezanson added this to the 1.12 milestone Jul 1, 2025
@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug backport 1.12 Change should be backported to release-1.12 labels Jul 1, 2025
@topolarity topolarity requested a review from Keno July 1, 2025 23:13
@JeffBezanson
Copy link
Member Author

JeffBezanson commented Jul 2, 2025 •
edited
Loading

So we basically need a custom interpreter here to handle the non-standard IR in ccall names, which I have tried to do but am flailing a bit 😄 It doesn't really need to be "correct" and its value is not used. It just needs to be enough for inference to tell which bindings are accessed.

So far I only have special support for nests of calls, which handles many cases like A.B.C but I wonder if there are any other syntaxes used out there?

JeffBezanson
JeffBezanson commented Jul 3, 2025
View reviewed changes
@KristofferC KristofferC mentioned this pull request Jul 8, 2025
Merged
60 tasks
@Keno Keno added the needs pkgeval Tests for all registered packages should be run with this change label Jul 10, 2025
@Keno
Copy link
Member

Keno commented Jul 10, 2025

One inline comment and I think we need to pkgeval this to see if there's any corner cases, but I think as a workaround, this seems reasonable.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests()

@Keno
Copy link
Member

Keno commented Jul 16, 2025

@KristofferC already pkgeval'd this in #58987

@JeffBezanson
Copy link
Member Author

This PR wasn't linked nor the tag removed. In any case that run was not too useful since there were assertion failures from the other PR.

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

3 packages crashed only on the current version.

  • The process was aborted: 1 packages
  • Invalid LLVM IR was generated: 1 packages
  • GC corruption was detected: 1 packages

3 packages crashed on the previous version too.

✖ Packages that failed

42 packages failed only on the current version.

  • Illegal method overwrites during precompilation: 1 packages
  • Package has test failures: 2 packages
  • Package tests unexpectedly errored: 2 packages
  • Tests became inactive: 1 packages
  • Test duration exceeded the time limit: 21 packages
  • Test log exceeded the size limit: 15 packages

1156 packages failed on the previous version too.

✔ Packages that passed tests

15 packages passed tests only on the current version.

  • Other: 15 packages

5369 packages passed tests on the previous version too.

~ Packages that at least loaded

16 packages successfully loaded only on the current version.

  • Other: 16 packages

3224 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

2 packages were skipped only on the current version.

  • Package could not be installed: 2 packages

903 packages were skipped on the previous version too.

@vtjnash
Copy link
Member

vtjnash commented Jul 17, 2025

Looks like up to 15 failures caused by this PR (CustomGaussQuadrature was in the last report as failing from this PR too)

Test log exceeded the size limit: 15 packages
Package	Version	Primary	Against	History (6-16 to 7-15)
Quadmath	v0.5.13	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
HyperDualNumbers	v4.0.10	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
JSOSolvers	v0.14.0	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
Lehmann	v0.2.7	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
GreenFunc	v0.2.6	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
ElectronGas	v0.2.6	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
TensorTrains	v0.12.1	fail	test	▇▇▅▅▇▅▇▅▅▇▅▅▇
IterativeRefinement	v0.2.1	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
HAdaptiveIntegration	v0.2.0	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
JacobiEigen	v1.0.1	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
RationalFunctionApproximation	v0.2.4	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
CaNNOLeS	v0.8.0	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
NEFTInterface	v0.1.0	fail	load	▅▅▅▅▅▅▅▅▅▅▅▅▅
CaratheodoryFejerApprox	v0.1.0	fail	test	▇▇▇▅▅▅▅▅▇▇▇▇▇
CustomGaussQuadrature	v1.0.6	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇

vtjnash
vtjnash reviewed Jul 17, 2025
View reviewed changes
Compiler/src/abstractinterpretation.jl
argtypes[i] = ai[].rt
i += 1
end
res = abstract_call(interp, ArgInfo(e.args, argtypes), sstate, sv)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, this call will occasionally corrupt the InferenceState structure for the current statement, since this is not actually the current statement (there might be some support code in return_type_tfunc for working around it though)

@KristofferC KristofferC mentioned this pull request Jul 22, 2025
Open
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@Keno Keno Awaiting requested review from Keno

Assignees
No one assigned
Labels
backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Milestone
1.12
Development

Successfully merging this pull request may close these issues.

ccall sometimes fails to statically evaluate arguments due to binding ages
4 participants
@JeffBezanson @Keno @nanosoldier @vtjnash